Skip to content

[GSoC 2017]Photometric Calibration. #1219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 53 commits into
base: 4.x
Choose a base branch
from
Open

[GSoC 2017]Photometric Calibration. #1219

wants to merge 53 commits into from

Conversation

nynyg
Copy link

@nynyg nynyg commented Jun 10, 2017 •
edited by alalek
Loading

Added Class PhotometricCalibrator.
Added method PhotometricCalibrator::validImgs.

First commit with dummy function and stab implementation to get familiar with PR procedure.

@grace-

TODO:

  • Tests (currently there are zero tests)
  • Remove absolute paths "/Users/Yelen/"
  • Avoid manual writing of YAML files
  • Avoid unconditional std::cout from module functions (need "verbose" parameter/flag/option)
  • Avoid manual calls of new/delete - use std::vector / cv::AutoBuffer instead

@nynyg nynyg force-pushed the photometric-calibration branch 2 times, most recently from 2db5273 to 515aa60 Compare June 20, 2017 10:19
@nynyg nynyg force-pushed the photometric-calibration branch from 515aa60 to d4d89c6 Compare June 20, 2017 10:51
grace-
grace- reviewed Jun 20, 2017
View reviewed changes
Copy link

@grace- grace- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree -- let's change pcalib --> photometric_calib everywhere so it's obvious what it is. Nice work!

modules/pcalib/include/opencv2/pcalib.hpp Outdated
#include <iostream>
#include <fstream>

namespace cv{ namespace pcalib{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space after cv

modules/pcalib/src/Reader.cpp Outdated
#include "opencv2/pcalib/Reader.hpp"
#include "precomp.hpp"

namespace cv{ namespace pcalib{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space after cv

modules/pcalib/src/pcalib.cpp Outdated
#include "precomp.hpp"
#include "opencv2/pcalib.hpp"

namespace cv{ namespace pcalib{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space after cv

modules/pcalib/src/precomp.hpp Outdated
#ifndef __OPENCV_PRECOMP_H__
#define __OPENCV_PRECOMP_H__

#include <opencv2/core.hpp>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use quotes consistently

modules/pcalib/include/opencv2/pcalib.hpp Outdated
#ifndef __OPENCV_PCALIB_HPP__
#define __OPENCV_PCALIB_HPP__

#include <opencv2/core.hpp>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use quotes

modules/pcalib/src/Reader.cpp Outdated
//

#include "opencv2/pcalib/Reader.hpp"
#include "precomp.hpp"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in cpp or test files precomp goes first

modules/pcalib/src/pcalib.cpp Outdated
//
//M*/

#ifndef __OPENCV_PCALIB_CPP__
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include guards only for header files

modules/pcalib/src/pcalib.cpp Outdated

#ifndef __OPENCV_PCALIB_CPP__
#define __OPENCV_PCALIB_CPP__
#ifdef __cplusplus
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also only for headers, do we need this/

@grace-
Copy link

grace- commented Jun 20, 2017

@vrabaud @vpisarev to review

@nynyg nynyg changed the title [GSoC 2017]Photometric Calibration. First commit with stab implementation. [GSoC 2017]Photometric Calibration. Jun 21, 2017
vrabaud
vrabaud reviewed Jun 26, 2017
View reviewed changes
Copy link
Contributor

@vrabaud vrabaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of basic things to correct.

modules/photometric_calib/README.md
@@ -0,0 +1,2 @@
Photometric Calibration
================================================
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the documentation ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

Thanks a lot for your comment!

modules/photometric_calib/include/opencv2/photometric_calib.hpp Outdated
class CV_EXPORTS PhotometricCalibrator : public Algorithm
{
public:
bool validImgs(std::vector <Mat> &inputImgs, std::vector<double> &exposureTime);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use tabs for indentation. Arguments should be const if they are const.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Thanks a lot for your comment!

modules/photometric_calib/include/opencv2/photometric_calib/Reader.hpp Outdated
class Reader
{
public:
Reader(std::string folderPath, std::string timesPath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const std::string &

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Thanks a lot for your comment!

modules/photometric_calib/src/Reader.cpp Outdated
@@ -0,0 +1,81 @@
//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget license at the top of every file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Vincent,

I think there is a shorter version (basically a link to http://opencv.org/license.html) and the complete OpenCV's license, which one should I use?

modules/photometric_calib/src/Reader.cpp Outdated
return (unsigned long)files.size();
}

void Reader::loadTimestamps(std::string timesFile)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the format of that file ? You should use OpenCV serialization for whatever you do.

modules/photometric_calib/src/Reader.cpp Outdated
{
Mat img = imread(files[i]);
CV_Assert(img.type() == CV_8U);
if(0 == i)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i==0

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Thanks a lot for your comment!

modules/photometric_calib/src/Reader.cpp Outdated
width = 0;
height = 0;

for(unsigned long i = 0; i < files.size(); ++i)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use size_t instead of unsigned long

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Vincent,

Should I also change

unsigned long getNumImages() const;
double getTimestamp(unsigned long id) const;
float getExposureTime(unsigned long id) const;

to

size_t getNumImages() const;
double getTimestamp(size_t id) const;
float getExposureTime(size_t id) const;

modules/photometric_calib/src/Reader.cpp Outdated
}
}

std::cout<<getNumImages()<<" imgases from"<<folderPath<<" loaded successfully!"<<std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use std::cout
Plus there is a typo: images.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Vincent, thanks a lot for your comments!

Can I use std::cerr to output some error info?

modules/photometric_calib/src/Reader.cpp Outdated
return timeStamps[id];
}

float Reader::getExposureTime(unsigned long id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this member function should be const.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Thanks a lot for your comment!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made 3 functions const:

unsigned long getNumImages() const;
double getTimestamp(unsigned long id) const; 
float getExposureTime(unsigned long id) const;

vrabaud
vrabaud reviewed Jun 27, 2017
View reviewed changes
Copy link
Contributor

@vrabaud vrabaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for cout/cerr, a better reviewer should answer. It seems it it used in the main library too, my bad.

modules/photometric_calib/include/opencv2/photometric_calib/Reader.hpp Outdated

double getTimestamp(unsigned long id) const;

float getExposureTime(unsigned long id) const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe getExposureDuration would be more explicit ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

modules/photometric_calib/include/opencv2/photometric_calib.hpp Outdated
#include <vector>
#include <string>
#include <iostream>
#include <fstream>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of those includes are useless in the header and should be in the .cpp

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

modules/photometric_calib/include/opencv2/photometric_calib/Reader.hpp Outdated
inline void loadTimestamps(const std::string &timesFile);

std::vector<String> files;
std::vector<double> timeStamps;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the format should be explained in the doc: what is that double ? Time since when ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Vincent,
Thanks for your comment.
doc means doxygen or?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I thought timestamps always means the Unix Timestamp since Jan 01 1970?

@nynyg
Copy link
Author

nynyg commented Jul 3, 2017

Result of removing Gamma function from the image:

Input Image:
input
Output Image:
image

@nynyg nynyg force-pushed the photometric-calibration branch from fc0f84e to 7c55a71 Compare July 4, 2017 07:21
@nynyg nynyg force-pushed the photometric-calibration branch from 7132110 to 2dd0ab0 Compare August 9, 2017 14:59
@nynyg nynyg force-pushed the photometric-calibration branch from 2dd0ab0 to d0ec626 Compare August 9, 2017 15:21
@alalek alalek mentioned this pull request Aug 17, 2017
Closed
@nynyg nynyg force-pushed the photometric-calibration branch from b010c10 to 15ca7a9 Compare August 20, 2017 14:09
@nynyg nynyg force-pushed the photometric-calibration branch from 49646df to ceb2af6 Compare August 21, 2017 10:51
@nynyg nynyg force-pushed the photometric-calibration branch 3 times, most recently from f692ab5 to 30a8c09 Compare August 21, 2017 14:18
@nynyg nynyg force-pushed the photometric-calibration branch from 30a8c09 to 6e97cc2 Compare August 21, 2017 14:47
@berak berak mentioned this pull request Oct 13, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vrabaud vrabaud vrabaud left review comments

@grace- grace- grace- left review comments

Assignees
No one assigned
Labels
GSoC
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nynyg @grace- @vrabaud @alalek